Skip to content

Raise explicitly on Python methods that are incompatible with lazy variables #1190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Feb 4, 2025

Notably changes the behavior of __bool__ to always raise. Before there was a hack based on whether a variable had been compared to something before.

This should avoid surprising behavior such as:

import pytensor.tensor as pt
x = pt.zeros(())
if x:
  print("Variable is nonzero!")  # Always triggered
else:
  print("Variable is zero")  # Never triggered

After this PR it raises

TypeError: TensorVariable cannot be converted to Python boolean. Call `.astype(bool)` for the symbolic equivalent.

The correct behavior as per the API standard is to raise in these cases: https://data-apis.org/array-api/latest/design_topics/lazy_eager.html

There is still surprising behavior with (in)equality checks, but we can't do anything about those as they are needed for Python hashing behavior. The default is x == y: True if x is y else False

import pytensor.tensor as pt
x = pt.zeros(())
if x == 0:
  print("Variable is zero!")  # Only triggered when x == x
else:
  print("Variable is nonzero")  # Always triggered

Related to #1189


📚 Documentation preview 📚: https://pytensor--1190.org.readthedocs.build/en/1190/

@ricardoV94 ricardoV94 force-pushed the explicitly_raise_on_py_incompatible_methods branch 3 times, most recently from 4e47797 to 457be84 Compare February 4, 2025 11:19
@ricardoV94 ricardoV94 force-pushed the explicitly_raise_on_py_incompatible_methods branch 2 times, most recently from fb92728 to c051841 Compare February 12, 2025 15:40
Copy link

codecov bot commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 82.53968% with 11 lines in your changes missing coverage. Please review.

Project coverage is 81.99%. Comparing base (3cdcfde) to head (04a6785).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pytensor/scalar/basic.py 50.00% 5 Missing ⚠️
pytensor/tensor/variable.py 78.57% 3 Missing ⚠️
pytensor/compile/nanguardmode.py 0.00% 0 Missing and 1 partial ⚠️
pytensor/tensor/rewriting/math.py 80.00% 0 Missing and 1 partial ⚠️
pytensor/tensor/rewriting/subtensor.py 0.00% 0 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (82.53%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1190      +/-   ##
==========================================
- Coverage   82.01%   81.99%   -0.02%     
==========================================
  Files         188      188              
  Lines       48561    48566       +5     
  Branches     8679     8677       -2     
==========================================
- Hits        39826    39823       -3     
- Misses       6572     6580       +8     
  Partials     2163     2163              
Files with missing lines Coverage Δ
pytensor/compile/function/pfunc.py 82.92% <100.00%> (ø)
pytensor/compile/function/types.py 80.66% <100.00%> (ø)
pytensor/scalar/loop.py 90.12% <100.00%> (ø)
pytensor/scalar/math.py 87.76% <100.00%> (ø)
pytensor/scan/basic.py 83.91% <100.00%> (ø)
pytensor/tensor/basic.py 91.36% <100.00%> (-0.02%) ⬇️
pytensor/tensor/conv/abstract_conv.py 76.07% <100.00%> (ø)
pytensor/tensor/math.py 92.01% <100.00%> (ø)
pytensor/tensor/rewriting/blas.py 89.64% <100.00%> (ø)
pytensor/tensor/rewriting/special.py 82.35% <100.00%> (ø)
... and 5 more

@ricardoV94 ricardoV94 marked this pull request as ready for review February 12, 2025 16:58
@ricardoV94 ricardoV94 requested a review from Armavica February 12, 2025 16:58
Copy link
Member

@Armavica Armavica left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove lines 779:783 of pytensor/scalar/basic.py, otherwise that looks good to me

…riables

Notably changes the behavior of `__bool__` to always raise. Before there was a hack based on whether a variable had been compared to something before.
@ricardoV94 ricardoV94 force-pushed the explicitly_raise_on_py_incompatible_methods branch from c051841 to 04a6785 Compare February 20, 2025 09:52
@ricardoV94
Copy link
Member Author

I think you can remove lines 779:783 of pytensor/scalar/basic.py, otherwise that looks good to me

Thanks, removed them

@ricardoV94 ricardoV94 merged commit c4fb0cf into pymc-devs:main Feb 20, 2025
72 of 73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants